Skip to content

Add warning when registering struct functions as workflows#1996

Closed
yuandrew wants to merge 4 commits intotemporalio:masterfrom
yuandrew:workflow-struct-warn
Closed

Add warning when registering struct functions as workflows#1996
yuandrew wants to merge 4 commits intotemporalio:masterfrom
yuandrew:workflow-struct-warn

Conversation

@yuandrew
Copy link
Copy Markdown
Contributor

@yuandrew yuandrew commented Jul 8, 2025

What was changed

Added a warning log saying we suggest only using structs for activity registration

Why?

Help prevent users from hitting potential NDEs

Checklist

  1. Closes SDK should warn when using structures to define a workflow #1986

  2. How was this tested:

N/A, only added a log statement

  1. Any docs updates needed?

@yuandrew yuandrew requested a review from a team as a code owner July 8, 2025 23:45
Quinn-With-Two-Ns
Quinn-With-Two-Ns reviewed Jul 9, 2025
View reviewed changes
Comment thread internal/internal_worker.go Outdated
aw.executionParams.DefaultVersioningBehavior == VersioningBehaviorUnspecified {
panic("workflow type does not have a versioning behavior")
}
_, ok := w.(WorkflowDefinitionFactory)
Copy link
Copy Markdown
Contributor

@Quinn-With-Two-Ns Quinn-With-Two-Ns Jul 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this effect the PHP SDK? I think this functionality is used by them

Copy link
Copy Markdown
Contributor Author

@yuandrew yuandrew Jul 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I misunderstood how the user was registering struct methods, the new commit should be correct, manually repro'd the issue.

Is PHP SDK built on top of Go?

Copy link
Copy Markdown
Contributor

@Quinn-With-Two-Ns Quinn-With-Two-Ns Jul 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is PHP SDK built on top of Go?

Yep

@Quinn-With-Two-Ns
Copy link
Copy Markdown
Contributor

Quinn-With-Two-Ns commented Jul 9, 2025

I am not sure we should be warning here, there is nothing inherently non deterministic in using a WorkflowDefinitionFactory (though it is mostly used by the PHP SDK)

It would only be non deterministic if a user didn't respect the contract to return a new instance with each call.

	// WorkflowDefinitionFactory factory for creating WorkflowDefinition instances.
	WorkflowDefinitionFactory interface {
		// NewWorkflowDefinition must return a new instance of WorkflowDefinition on each call.
		NewWorkflowDefinition() WorkflowDefinition
	}

Quinn-With-Two-Ns
Quinn-With-Two-Ns reviewed Jul 10, 2025
View reviewed changes
Comment thread internal/internal_worker.go Outdated
}
fnName := runtime.FuncForPC(reflect.ValueOf(w).Pointer()).Name()
if strings.HasSuffix(fnName, "-fm") {
aw.logger.Warn("It is recommended to only use struct methods to define activity functions. In some cases, struct methods as workflows may cause NDE errors.")
Copy link
Copy Markdown
Contributor

@Quinn-With-Two-Ns Quinn-With-Two-Ns Jul 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit could we log the function they tried to register here too?

cretz
cretz reviewed Jul 10, 2025
View reviewed changes
Comment thread internal/internal_worker.go Outdated
}
fnName := runtime.FuncForPC(reflect.ValueOf(w).Pointer()).Name()
if strings.HasSuffix(fnName, "-fm") {
aw.logger.Warn("Registering workflow" + fnName + ". It is recommended to only use struct methods to define activity functions. In some cases, struct methods as workflows may cause NDE errors.")
Copy link
Copy Markdown
Contributor

@cretz cretz Jul 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pedantic, but would avoid the acronym NDE in user-facing contexts

Copy link
Copy Markdown
Contributor Author

@yuandrew yuandrew Jul 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for the suggestion :)

cretz
cretz approved these changes Jul 11, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@cretz cretz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(approving because I technically left a comment here, but my approval on Go issues these days can be ignored heh)

@yuandrew
Copy link
Copy Markdown
Contributor Author

yuandrew commented Jul 11, 2025

Decided internally that since this spams our test logs (our test workflows are all methods under the same struct), for now we'll add something in the documentation warning against this.

Our tests can be changed to not share the same struct, but that's a rather large refactor.

@yuandrew yuandrew closed this Jul 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Quinn-With-Two-Ns Quinn-With-Two-Ns Quinn-With-Two-Ns left review comments

+1 more reviewer

@cretz cretz cretz approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

SDK should warn when using structures to define a workflow

3 participants

@yuandrew @Quinn-With-Two-Ns @cretz